Greptile Summary

This PR replaces ad-hoc grep-based transitive dependency checking with a deterministic AST-driven inventory script (audit_package_dependencies.py), and refactors the per-finding workflow state management into a dedicated helper (manage_agentic_ci_attempts.py) that groups batched findings by their shared (pr_number, branch).

• Deterministic inventory: audit_package_dependencies.py walks each package's source tree via the Python AST, resolves modules to distributions using packages_distributions() or installed metadata, computes transitive closure over declared dependencies, and classifies gaps as low (covered by a workspace dep's closure) vs high (no guarantor) — replacing the previous ad-hoc grep recipe.
• Group-based validation: the scope gate and lockfile gate now iterate (pr_number, branch) groups instead of individual attempted_fixes entries; a single rejection closes one PR and marks every associated finding abandoned atomically.
• uv.lock gate: dependency PRs are now required to include a regenerated uv.lock; a missing lockfile or one that drifts after make install-dev triggers immediate rejection with a distinct abandonment marker.

Important Files Changed

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant W as Workflow (agentic-ci-daily)
    participant AP as audit_package_dependencies.py
    participant MA as manage_agentic_ci_attempts.py
    participant S as runner-state.json
    participant G as git / gh

    Note over W,AP: Build dependency inventory (dependencies suite only)
    W->>AP: --output /tmp/dependency-inventory.json
    AP-->>W: "JSON {packages[].missing, unresolved_modules}"

    Note over W,MA: Scope gate
    W->>MA: groups --state ... --prior ...
    MA->>S: read state + prior counts
    MA-->>W: "OPEN_GROUPS [{pr_number, branch, finding_ids}]"

    loop For each GROUP
        W->>G: validate files (allowlist + LOC excl. uv.lock)
        alt Scope gate fails
            W->>G: gh pr close + delete branch
            W->>MA: abandon --finding-ids ... --marker gate_violation
            MA->>S: mark all finding IDs abandoned
        end
    end

    Note over W,MA: Lockfile gate (dependencies only, if scope gate passed)
    W->>MA: groups --state ... --prior ...
    MA-->>W: OPEN_GROUPS (only still-open entries)

    loop For each GROUP
        W->>G: git checkout $BRANCH
        alt Checkout failed
            W->>MA: abandon --marker lockfile_checkout_failed
        else uv.lock missing from diff
            W->>MA: abandon --marker lockfile_missing
        else make install-dev OK
            W->>G: git diff --quiet -- uv.lock
            alt uv.lock changed after install
                W->>MA: abandon --marker lockfile_not_committed
            end
        else make install-dev failed
            W->>MA: abandon --marker lockfile_verification_failed
        end
    end

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant W as Workflow (agentic-ci-daily)
    participant AP as audit_package_dependencies.py
    participant MA as manage_agentic_ci_attempts.py
    participant S as runner-state.json
    participant G as git / gh

    Note over W,AP: Build dependency inventory (dependencies suite only)
    W->>AP: --output /tmp/dependency-inventory.json
    AP-->>W: "JSON {packages[].missing, unresolved_modules}"

    Note over W,MA: Scope gate
    W->>MA: groups --state ... --prior ...
    MA->>S: read state + prior counts
    MA-->>W: "OPEN_GROUPS [{pr_number, branch, finding_ids}]"

    loop For each GROUP
        W->>G: validate files (allowlist + LOC excl. uv.lock)
        alt Scope gate fails
            W->>G: gh pr close + delete branch
            W->>MA: abandon --finding-ids ... --marker gate_violation
            MA->>S: mark all finding IDs abandoned
        end
    end

    Note over W,MA: Lockfile gate (dependencies only, if scope gate passed)
    W->>MA: groups --state ... --prior ...
    MA-->>W: OPEN_GROUPS (only still-open entries)

    loop For each GROUP
        W->>G: git checkout $BRANCH
        alt Checkout failed
            W->>MA: abandon --marker lockfile_checkout_failed
        else uv.lock missing from diff
            W->>MA: abandon --marker lockfile_missing
        else make install-dev OK
            W->>G: git diff --quiet -- uv.lock
            alt uv.lock changed after install
                W->>MA: abandon --marker lockfile_not_committed
            end
        else make install-dev failed
            W->>MA: abandon --marker lockfile_verification_failed
        end
    end

_{Reviews (5): Last reviewed commit: "Merge branch 'main' into andreatgretel/f..." | Re-trigger Greptile}

Code Review: PR #787 — fix(agentic-ci): improve dependency PR quality

Summary

This PR reworks the agentic-CI dependencies suite to make its dependency audit deterministic and its fix PRs safer:

• New script scripts/audit_package_dependencies.py (227 lines): builds an AST-based import/dependency inventory. For each workspace package it compares imported third-party modules against static + dynamic (uv-dynamic-versioning) declarations, resolves modules to distributions, and classifies missing declarations by severity.
• Severity model shift: a gap is low when the imported distribution is transitively guaranteed by a mandatory declared dependency (guaranteed_by), and high only when nothing guarantees it (true standalone-install breakage).
• New tests test_dependency_audit.py (91 lines) exercising the three severity paths (transitively-guaranteed low, uncovered high, external-transitive low).
• Recipe/policy (dependencies/recipe.md, _fix-policy.md): consume the inventory JSON instead of ad-hoc grep; batch up to 3 gaps per target package; require dependency PRs to commit a regenerated, clean uv.lock.
• Workflow (agentic-ci-daily.yml): builds the inventory before the run, widens the dependencies allowlist to include uv.lock, and adds a lockfile gate that rejects PRs missing uv.lock or where make install-dev mutates it.

Change is well-scoped and matches the stated validation (991 passed, ruff clean, uv lock --check).

Findings

Correctness — script looks sound

• The guaranteed_by closure logic is correct: installed_requirements filters environment markers via evaluate({"extra": ""}), so only mandatory transitive requirements count toward the "guaranteed" (low-severity) classification — extras/optional deps correctly do not downgrade severity. This is the right conservative direction (unknown → high → more scrutiny).
• Workspace siblings are handled by short-circuiting requirements_for to the sibling's own declared set, so config→engine jinja2 correctly resolves to guaranteed_by: [data-designer-config]. The test confirms this.
• Relative imports are excluded (node.level == 0), stdlib is filtered via sys.stdlib_module_names, and data_designer is skipped — all correct for a third-party inventory.
• dependency_closure is an iterative DFS with a closure visited-set, so cyclic dependency graphs terminate safely.

Nit (low) — shadowing of the imported distribution

• audit_repository rebinds the name distribution as a loop variable in two places (distribution = resolve_distribution(...) and for distribution, usage in sorted(...)), shadowing the module-level from importlib.metadata import ... distribution. It's not a bug — distribution() is only called inside installed_requirements, which has its own scope — but it hurts readability and is the kind of thing a future edit could trip over. Consider renaming the locals to dist/distribution_name.

Determinism caveat (informational, not a defect)

• The default path uses packages_distributions() and installed_requirements(), which reflect the installed environment, and marker evaluation uses the running platform. A dependency required only on another platform could be counted as guaranteed on some runners and not others. This is conservative (worst case = flagged high) and acceptable, but worth a one-line comment in the script noting the inventory is environment-relative.

unresolved_modules / resolve_distribution ambiguity

• resolve_distribution returns None (→ unresolved_modules) when a module maps to multiple candidate distributions none of which are declared. Good conservative behavior, and the recipe correctly routes unresolved_modules to manual review ("Never guess a package name"). No direct unit test covers the multi-candidate/unresolved branch or the MODULE_DISTRIBUTION_OVERRIDES path — worth adding for completeness given the audit's authority over auto-generated PRs, but not blocking.

Workflow gate — logic is correct

• The uv.lock-present check (git diff --name-only origin/main...HEAD -- uv.lock | grep -qx 'uv.lock') and the post-install-dev cleanliness check (git diff --quiet -- uv.lock) together enforce the "committed, resolves-clean" contract. Both rejection branches call abandon_open_attempt and close the PR/branch consistently with the surrounding error handling. Note grep -qx 'uv.lock' treats . as a regex wildcard, but since git only ever emits the literal path this is harmless.

Docs/recipe consistency

• _fix-policy.md batching now keys on "target package, test target, and branch type" and the recipe/allowlist/constraints all consistently add uv.lock. The severity table, report template, and eligibility notes are internally consistent with the new guaranteed_by/severity fields emitted by the script. No drift spotted.

Test coverage

• The three included tests map cleanly onto the three severity outcomes and use injected distributions/requirements maps to stay hermetic (no reliance on the installed env) — good design. Gaps: no test for unresolved_modules, the override map, or the unused-dependency reporting path.

Structural Impact

Structural Impact (graphify, 2.3s)

Risk: LOW (localized change)

• 2 Python files, 0 AST entities, 0/81 clusters

Verdict

Approve (non-blocking nits). The new script is correct, hermetically testable, and the severity model is a genuine improvement over the prior grep-based transitive check. The workflow lockfile gate closes a real failure mode (deps that pass the old lockfile but don't resolve standalone). None of the findings block merge:

• Rename the shadowed distribution locals for readability.
• Consider a short comment noting the inventory is environment-relative.
• Optionally add tests for unresolved_modules and the override path.

All changes are confined to .agents/, .github/workflows/, scripts/, and test files — no impact on the shipped data_designer packages or the interface→engine→config import direction.

Thanks for putting this together, @andreatgretel!

Summary

This PR replaces the dependency suite's grep-based discovery with a structured AST/import-metadata inventory, distinguishes guaranteed declaration gaps from uncovered ones, batches compatible fixes, and requires dependency PRs to carry a clean regenerated lockfile. The implementation matches the stated intent, but two edge cases in the new classification and batching behavior are worth fixing before merge.

Findings

Warnings — Worth addressing

scripts/audit_package_dependencies.py:42 — Declaration parsing drops markers and selected extras

• What: declared_dependencies() reduces every requirement to only its normalized distribution name. That discards project-level environment markers and selected extras before dependency_closure() runs; installed_requirements() then evaluates all transitive markers with extra="".
• Why: This can classify gaps in both directions incorrectly. For example, a package declaring pydantic[email] is guaranteed to install email-validator, but an email_validator import would be reported as uncovered/high. Conversely, an inactive conditional declaration such as foo; python_version < "3.11" would still be treated as guaranteed on newer Python. That undermines the low/high distinction this PR is introducing.
• Suggestion: Preserve each declaration's marker and selected extras (using packaging.Requirement for ordinary requirements, with the existing dynamic-version template handled separately), evaluate project markers for the runner environment, and evaluate transitive requirements once for the base dependency plus each explicitly selected extra. Please add focused tests for an enabled extra and an inactive project marker.

.agents/recipes/dependencies/recipe.md:181 — Batched PRs are still validated once per finding

• What: The recipe now puts up to three findings in one PR and records one attempted_fixes entry per dependency, but both post-fix gates still iterate those entries independently. In particular, the lockfile gate runs make install-dev once per finding even when all entries point to the same branch/PR, and each rejection updates only the current finding ID.
• Why: A three-finding batch can run the same expensive validation three times. More importantly, if an early repetition passes and a later repetition fails transiently, the shared PR is closed while the earlier finding IDs remain recorded as open until a future reconciliation run, violating the policy's atomic-state guarantee.
• Suggestion: Group new entries by (pr_number, branch), validate each pushed diff once, and apply the result atomically to every finding ID in that group. The scope gate can use the same grouping to avoid redundant fetch/diff work; a small fixture covering a multi-finding rejection would lock the state transition down.

What Looks Good

• The AST walk, stable sorting, conservative unresolved-module handling, and injected metadata maps make the inventory predictable and testable.
• The three tests cleanly cover workspace-transitive, external-transitive, and genuinely uncovered severity paths without depending on the host environment.
• The recipe, allowlist, and workflow agree on committing uv.lock, and the presence-plus-cleanliness checks close the stale-lockfile failure mode.

Verdict

Needs changes — preserve requirement markers/extras in closure classification, and make batched PR validation/state updates operate once per shared PR rather than once per finding.

This review was generated by an AI assistant.

Addressed the review feedback:

• Dependency declarations now preserve and evaluate environment markers and selected extras when computing transitive guarantees. Dynamic {{ version }} dependencies remain supported.
• Post-fix scope and lockfile gates now group attempts by (pr_number, branch), validate each shared diff once, and atomically abandon every associated finding on rejection.
• Renamed the shadowing distribution locals.
• Added focused tests for enabled extras, inactive project markers, batched grouping, and multi-finding rejection.

Circling back on this one, @andreatgretel — took another pass now that it's in shape. Overall it's holding up well: the AST inventory reads cleanly, ruff is happy, the three new tests pass, and the workflow YAML parses. A couple of things are still worth a look before merge, but nothing that changes the shape of the PR.

Still worth addressing

uv.lock counts against the 50-LOC net cap (_fix-policy.md:14, agentic-ci-daily.yml:372)
The suite now requires uv.lock in every dependency PR (the new lockfile gate closes any that omit it), yet the localized-fix bar is still "≤50 LOC net" and the scope gate's LOC_DELTA is computed across the whole diff — uv.lock included. For the eligible case (a gap already guaranteed_by a required sibling, so the dist is resolved and locked) the lock delta is usually a line or two and this survives. But batching up to 3 deps, or uv re-serializing a block, could tip the combined diff past 50 lines and get the PR silently closed — leaving the two gates mutually unsatisfiable. Could we confirm a realistic 3-dep regeneration stays under 50 net lines, or just exclude uv.lock from LOC_DELTA (... -- ':(exclude)uv.lock') and lean on the file-count/allowlist checks?

guaranteed_by/severity depend on the runner's environment (audit_package_dependencies.py:97)
installed_requirements evaluates markers with only {"extra": ""}, so sys_platform/python_version markers resolve against the current runner. A platform-conditional transitive dep could downgrade a genuinely-missing dependency from high to low on CI. Not blocking given the recipe re-confirms severity, but a one-line comment noting marker evaluation is environment-relative would keep future readers from over-trusting guaranteed_by.

Minor / optional

• audit_package_dependencies.py:167,180 — the local distribution shadows the imported importlib.metadata.distribution. Harmless today (only called from installed_requirements), but a trap for a future edit; dist / distribution_name would be safer.
• test_dependency_audit.py — the severity paths are well covered; the unresolved_modules, MODULE_DISTRIBUTION_OVERRIDES, and ambiguous multi-candidate branches of resolve_distribution aren't yet. A couple of small cases there would close the gap.
• audit_package_dependencies.py:62 — imported_modules counts TYPE_CHECKING/guarded imports as runtime imports (intentional, since the recipe re-verifies). A brief comment saying so would save the next reader a double-take.

Still looking good

• The deterministic AST + metadata inventory is a genuine upgrade over the old grep, and driving the low/high split off a transitive closure cleanly separates hygiene from real standalone-install breaks.
• Hermetic tests via injected module_distributions/requirement_map — no dependency on the installed env.
• The lockfile gate reuses the scope gate's snapshot-based "grew this run" selector and fails closed on checkout/fetch failures, consistent with the atomicity model.
• Spec and enforcement stay in sync — uv.lock added to both the _fix-policy.md allowlist and the workflow regex.

Verdict

Needs changes — no correctness issues, but the uv.lock-vs-50-LOC interaction is worth confirming or resolving before this goes live, since it can quietly make dependency fixes unlandable. Everything else is take-it-or-leave-it.

This review was generated by an AI assistant.

Addressed the follow-up feedback:

• Excluded generated uv.lock changes from the dependency-suite LOC cap while retaining file-count and allowlist enforcement.
• Documented that requirement markers are evaluated relative to the audit environment.
• Added coverage for ambiguous module mappings and distribution overrides.
• Documented the intentional inclusion of guarded and TYPE_CHECKING imports.